Skip to content

Automatically install or update ocaml-lsp-server #1725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 34 commits into from
Apr 3, 2025
Merged

Automatically install or update ocaml-lsp-server #1725

merged 34 commits into from
Apr 3, 2025

Conversation

PizieDust
Copy link
Contributor

@PizieDust PizieDust commented Feb 10, 2025 •
edited
Loading

Currently, when ocaml-lsp-server is not found in a sandbox, the language server displays an error and fails to start.
This PR improves users experience by providing a UI that asks if they will like to install or update ocaml-lsp-server or select another sandbox.

If the user wants to select another sandbox, a quickpick will be displayed with various switches.
If the user decides to install ocaml-lsp-server, it will be installed in the background and they can continue with their activities in the editor.

This PR will streamline the process for complete beginners and also reduce the time to terminal for experienced users who often will have to switch to the terminal to install ocaml-lsp-server in their sandbox.

cc @voodoos

@PizieDust PizieDust requested a review from xvw February 10, 2025 11:01
@smorimoto smorimoto added type: feature New feature or request type: improvement labels Feb 10, 2025
voodoos
voodoos reviewed Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pizie, this looks quite reasonable and works great.

I understand now the "notifications" you were talking about earlier:

image

Maybe we could improve the message of the "Installing sandbox packages" to also mention the name of the packages so that we could remove the additional "Installing the latest release of...." notification ?

@PizieDust PizieDust requested a review from voodoos February 11, 2025 07:58
@PizieDust
Copy link
Contributor Author

Thanks Pizie, this looks quite reasonable and works great.

I understand now the "notifications" you were talking about earlier:

image Maybe we could improve the message of the "Installing sandbox packages" to also mention the name of the packages so that we could remove the additional "Installing the latest release of...." notification ?

Thank you. This is done in the latest commit.

@voodoos
Copy link
Collaborator

voodoos commented Feb 11, 2025 •
edited
Loading

I have one concern with the current behavior: what install package does is:

$ update --switch=5.3.0
$ install --switch=5.3.0 -y ocaml-lsp-server

There is nothing fundamentally wrong with that, but I think the second command will also upgrade every upgradable package in the switch. This might be something we want to skip to prevent potentially breaking user projects ? (not sure if that is doable however) 

edit: Just tested it, and opam is not upgrading the switch by default, so we are all good here.

voodoos
voodoos reviewed Feb 11, 2025
View reviewed changes
@PizieDust PizieDust changed the title Automatically install ocaml-lsp-server Automatically install or update ocaml-lsp-server Feb 18, 2025
voodoos
voodoos reviewed Feb 18, 2025
View reviewed changes
@PizieDust PizieDust requested a review from voodoos February 19, 2025 13:57
voodoos
voodoos reviewed Feb 28, 2025
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. I have a few suggestions left for the upgrades 🙂

src/extension_instance.ml Outdated

let upgrade_ocaml_lsp_server sandbox latest_version =
let open Promise.Syntax in
let+ () = Sandbox.install_packages sandbox [ "ocaml-lsp-server=" ^ latest_version ] in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be much better to modify Sandbox.upgrade_packages to accept a list of packages to upgrade and use that.

That way you wouldn't have to carry the 'latest_version' around and concatenate strings to try to install the latest version. Opam knows how to do that better :-)

You might want to rename Sandbox.upgrade_packages to Sandbox.upgrade_all_packages and then introduce the specialized Sandbox.upgrade_packages to work on a given list of packages.

voodoos
voodoos reviewed Mar 21, 2025
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Pizie, that looks quite close to being ready :-)

@PizieDust PizieDust requested a review from voodoos March 27, 2025 13:52
voodoos
voodoos approved these changes Apr 3, 2025
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just made a few more remarks, but once fixed I think this is ready to go.

In my opinion this hints toward a long-awaited refactoring of how we check for available commands. It could be unified so that we have the same behavior for every command: Not available ? Then do you want to upgrade ? Nice. Arg.

But that's the subject of another PR.

@smorimoto smorimoto requested a review from voodoos April 3, 2025 12:02
voodoos
voodoos reviewed Apr 3, 2025
View reviewed changes
voodoos
voodoos approved these changes Apr 3, 2025
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pizie, I think the current state is all-right for introducing this new functionality.

We might want to do some refactoring / cleanup of the mechanism to detect missing features in the future.

@smorimoto smorimoto merged commit 56dfec6 into ocamllabs:master Apr 3, 2025
7 checks passed
@PizieDust PizieDust deleted the auto_lsp branch April 4, 2025 06:01
PizieDust added a commit to PizieDust/vscode-ocaml-platform that referenced this pull request Apr 18, 2025
@PizieDust @voodoos
Automatically install or update ocaml-lsp-server (ocamllabs#1725)
45d03f3 
* check if ocaml-lsp-server is installed

* suggest to install ocaml-lsp-server

* command to install ocaml-lsp-server

* linting

* add changelog

* remove type annotations

* Update src/extension_instance.ml

Co-authored-by: Ulysse <[email protected]>

* Update src/extension_commands.ml

Co-authored-by: Ulysse <[email protected]>

* Update package.json

Co-authored-by: Ulysse <[email protected]>

* displayy packages in installation notification

* lint

* Update package.json

Co-authored-by: Ulysse <[email protected]>

* modify to include latest version

* prompt user to update ocaml-lsp-server if outdated

* correct sandbox spelling

* better notification messages

* ask user to upgrade or do nothing

* correct typo

* lint

* Update src/sandbox.ml

Co-authored-by: Ulysse <[email protected]>

* use variants and not strings

* pass a list of packages to upgrade

* update opem upgrade to take a an optional list of packages

* revert

* remove latest_version from function

* use consistent annotations

* remove latest_Version

* make suggets_to_update_lsp public

* suggest to upgrade lsp when using old version

* wait for promises to resolve

* suggest to upgrade ocaml_lsp in ocaml_lsp.is_version_up_to_date

* Update src/ocaml_lsp.ml

Co-authored-by: Ulysse <[email protected]>

* Update src/extension_instance.ml

Co-authored-by: Ulysse <[email protected]>

* better error handling

---------

Co-authored-by: Ulysse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoos voodoos voodoos approved these changes

@xvw xvw Awaiting requested review from xvw

@smorimoto smorimoto Awaiting requested review from smorimoto

Assignees
No one assigned
Labels
type: feature New feature or request type: improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PizieDust @voodoos @xvw @smorimoto